Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Removed attach command #3409

Merged
merged 5 commits into from
Aug 28, 2024
Merged

feat: Removed attach command #3409

merged 5 commits into from
Aug 28, 2024

Conversation

jirihnidek
Copy link
Contributor

  • Card ID: CCT-425

Attach command

  • Removed attach command
  • Removed --auto-attach and --autosubscribe CLI options of
    register command
  • Removed --servicelevel CLI option of register command too,
    because this CLI option was possible to use only with
    --auto-attach CLI option. Thus, it does not make sense
    to keep this option
  • Removed unit tests for attach command
  • Modified unit tests for register command. Only SCA mode
    is considered.
  • Modified man pages

D-Bus

  • Removed interface: com.redhat.RHSM1.Attach
  • Removed AutoAttach and PoolAttach
  • Modified Register method
  • Removed unit tests related to removed methods
  • Modified unit tests of register D-Bus methods

Attach service

  • Removed attach service
  • Removed unit tests related to attach service

@cnsnyder cnsnyder requested review from a team and cnsnyder and removed request for a team June 3, 2024 12:24
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 3, 2024 •
edited
Loading

Coverage

Coverage (computed on Fedora latest) •
FileStmtsMissCoverMissing
__init__.py00100% 
rhsmlib/dbus/objects
   __init__.py80100% 
   register.py2016269%35, 88–89, 102–103, 113–114, 116–117, 127–128, 130, 144, 147, 160, 163, 202, 211, 249–250, 257, 275, 283, 286, 329–331, 333–334, 353–356, 358–360, 362, 364, 366, 381, 385, 402–407, 409–411, 413, 415, 427–431, 433–435, 437, 439
subscription_manager
   managercli.py481275%22, 80–82, 84, 86–91, 95
subscription_manager/cli_command
   cli.py2062886%64, 68, 124, 181, 185, 187, 230, 269, 280–282, 296–298, 321, 341, 367, 376–377, 381–382, 398, 400–401, 403–404, 409, 417
   register.py2044577%120, 122, 165–167, 169–170, 173, 184, 188, 195–198, 202–203, 207–208, 219, 247, 251–253, 262, 266, 292, 308–309, 345, 374, 382, 397–399, 408, 439, 449–450, 457–458, 463–467
subscription_manager/scripts
   rhsm_service.py15150%17, 19–20, 23, 26–28, 31–33, 43–45, 48–49
TOTAL18014453874% 

Tests Skipped Failures Errors Time
2534 14 💤 0 ❌ 0 🔥 34.229s ⏱️

@jirihnidek jirihnidek force-pushed the jhnidek/remove_attach branch 2 times, most recently from 61b4637 to 371a666 Compare June 3, 2024 15:31
m-horky
m-horky requested changes Jun 6, 2024
View reviewed changes
man/subscription-manager.8 Show resolved Hide resolved
man/subscription-manager.8 Show resolved Hide resolved
@jirihnidek jirihnidek force-pushed the jhnidek/remove_attach branch from 371a666 to b667da0 Compare June 19, 2024 10:11
m-horky
m-horky approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@m-horky m-horky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still many references to attaching in the code, but that would be out of scope for the command. However, I found

subscription-manager/src/subscription_manager/cli_command/cli.py

Lines 118 to 135 in b667da0

def _print_ignore_auto_attach_message(self):
"""
This message is shared by attach command and register command, because
both commands can do auto-attach.
:return: None
"""
owner = get_current_owner(self.cp, self.identity)
# We displayed Owner name: `owner_name = owner['displayName']`, but such behavior
# was not consistent with rest of subscription-manager
# Look at this comment: https://bugzilla.redhat.com/show_bug.cgi?id=1826300#c8
owner_id = owner["key"]
print(
_(
"Ignoring the request to auto-attach. "
'Attaching subscriptions is disabled for organization "{owner_id}" '
"because Simple Content Access (SCA) is enabled."
).format(owner_id=owner_id)
)

that I believe should be part of this patch.

Otherwise LGTM. I won't block this PR on this change, we'll find it later during cleanup work.

@jirihnidek jirihnidek force-pushed the jhnidek/remove_attach branch 2 times, most recently from b261e4f to 02b3c62 Compare June 25, 2024 15:16
@ptoscano
Copy link
Contributor

This must not be merged before candlepin/subscription-manager-cockpit#68 is merged, otherwise the cockpit tests will fail (like they currently do).

@jirihnidek jirihnidek force-pushed the jhnidek/remove_attach branch from 02b3c62 to 34f9232 Compare August 2, 2024 14:33
@DuckBoss DuckBoss mentioned this pull request Aug 16, 2024
Merged
@jirihnidek @ptoscano
feat: Removed attach command and CLI option related to attach
ee03946 
* Card ID: CCT-425
* Removed attach command
* Removed --auto-attach and --autosubscribe CLI options of
  register command
* Removed --servicelevel CLI option of register command too,
  because this CLI option was possible to use only with
  --auto-attach CLI option. Thus, it does not make sense
  to keep this option
* Removed unit tests for attach command
* Modified unit tests for register command. Only SCA mode
  is considered.
@jirihnidek @ptoscano
feat: Removed D-Bus methods related to attach
8b24f97 
* Card ID: CCT-425
* Removed interface: com.redhat.RHSM1.Attach
* Removed AutoAttach and PoolAttach
* Modified Register method
* Removed unit tests related to removed methods
* Modified unit tests of register D-Bus methods
@jirihnidek @ptoscano
feat: Removed attach service
792479a 
* Card ID: CCT-425
* Removed attach service
* Removed unit tests related to attach service
@jirihnidek @ptoscano
feat: Remove references on auto-attach in man page
b6227e8 
* Card ID: CCT-425
* Removed relevant references on auto-attach in man pages
  of subscription-manager
* Note: references on auto-atach (autoheal) was not removed,
  because it is different command and it only enable/disable
  autoheal option of rhsmcertd
@jirihnidek @ptoscano
feat: Remove attach from bash completion script
3d1e806 
* Card ID: CCT-425
* Attach command was removed from bash complition script as well
  --auto-attach CLI option and --autosubscribe from register
  command
@ptoscano ptoscano force-pushed the jhnidek/remove_attach branch from 34f9232 to 3d1e806 Compare August 28, 2024 13:27
ptoscano
ptoscano approved these changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK, so I can merge this.

There are few more things related to these changes to fix:

  • etc-conf/dbus/system.d/com.redhat.RHSM1.conf: traces of the removed D-Bus interface
  • src/rhsmlib/dbus/constants.py: the now unused ATTACH_INTERFACE and ATTACH_DBUS_PATH constants

One or more followup PRs would be nice, thanks!

@ptoscano ptoscano merged commit 00deef2 into main Aug 28, 2024
15 checks passed
@ptoscano ptoscano deleted the jhnidek/remove_attach branch August 28, 2024 13:52
@Glutexo Glutexo mentioned this pull request Nov 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-horky m-horky m-horky approved these changes

@ptoscano ptoscano ptoscano approved these changes

@cnsnyder cnsnyder Awaiting requested review from cnsnyder cnsnyder was automatically assigned from candlepin/client-tools

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jirihnidek @ptoscano @m-horky